Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gazelle): _actually_ don't write narrow dependencies. #1683

Closed
wants to merge 5 commits into from
Closed

fix(gazelle): _actually_ don't write narrow dependencies. #1683

wants to merge 5 commits into from

Conversation

aryamccarthy
Copy link

@aryamccarthy aryamccarthy commented Jan 11, 2024

Sorry about #1681 - I had uploaded an older version. This one contains the correct behavior, in line with what I had written.

The goal is to inherit visibility from the BUILD file's default_visibility (or whatever defaults exist higher in the tree).

I'm happy to discuss the issue further at #1682 if you have apprehensions about this change.

@aryamccarthy
Copy link
Author

Note that every failing test right now is for the same reason. They look like this:

          	py_binary(
          	    name = "main",
          	    srcs = ["main.py"],
        - 	    visibility = ["//:__subpackages__"],
          	    deps = ["@pip//:pandas"],
          	)

This is because the auto-generated visibility lines aren't produced by this PR. That's the intended behavior, but grep-ing for the test names didn't turn anything up. Otherwise, I'd update the tests myself.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests that are failing are in gazelle/python/testdata if I remember correctly.

@@ -212,7 +212,6 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
}

parser := newPython3Parser(args.Config.RepoRoot, args.Rel, cfg.IgnoresDependency)
visibility := fmt.Sprintf("//%s:__subpackages__", pythonProjectRoot)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is desirable in usecases where you have a monorepo, where you have many requirements.txt files that you don't want to mix. This change would be a regression in that front.

The go plugin for gazelle has directives to control the visibility as is documented in https://github.com/bazelbuild/bazel-gazelle, maybe introducing a directive would be useful here as well?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this PR you can still get the old behavior by adding this to your Python root BUILD file:

package(default_visibility = ["//" + package_name() + ":__subpackages__"])

There's some more context in this previous PR that accidentally included the wrong change but the right PR description.

@aignas
Copy link
Collaborator

aignas commented Mar 1, 2024

Let me know if we can close this since #1784 got merged. i think that should address your use-case.

@aignas aignas closed this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants